-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Document how to handle compatibility breakages #9158
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot! Sounds good so far, my feedback is very nitpicky 😅
Regarding an example, maybe we should use a real one that occurred? Then we don't need to make up things. It probably illustrates the principle better than an abstract one with "arg1", "arg2" etc.
Thanks for starting on this! I look forward to linking to it in the future :-) |
63f7dbf
to
f3e28b0
Compare
Added a concrete example, will go back over the style review as well, and the wording in the new example code isn't perfect, any feedback would be appreciated there |
f3e28b0
to
dcb5533
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This is looking really great so far.
c14e505
to
04ed066
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some suggestions, but aside from a couple typo/style nitpicks, I think this could be merged as is, and improved later. It's already a much needed and sufficient addition to our contributor docs.
So you've added a new parameter to a method, changed the return type, | ||
changed the type of a parameter, or changed its default value, | ||
and now the automated testing is complaining about compatibility breakages? | ||
|
||
Not to worry, there's a system to handle just that! | ||
|
||
Compatibility is handled in a few different ways. In GDScript this is simple, | ||
new arguments are usually given default values when added, so existing scripts | ||
won't break in such cases. | ||
|
||
However in other cases, like GDExtension and C#, the exact way the method is | ||
declared matters and must be matched exactly. This is handled by creating | ||
compatibility methods that then get mapped to the new versions of these | ||
methods. All of this happens *almost* automatically, there are just a few | ||
things you need to do. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel we need to call out the different types of "compatibility breakages" more explicitly:
- For GDScript, adding a new parameter at the end of a method prototype with a default value does not break compatibility, but it does for GDExtension and C#, as described here. This is the only "compat breakage" that is ok to do in general.
- All other compatibility breakages break GDScript even if compat methods are added, so they can only be done exceptionally with approval for the release management team.
Not sure how to clarify this, with the introduction already being a bit long. I think my main issue is that the light hearted "no worry, there's a system that lets you break compat" may give the wrong impression about what kind of changes are allowed by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed will see what I can come up with, maybe we can cut this down and make a separate page discussing the general topic of compatibility focused on the abstract instead of the specific
ClassDB::bind_compatibility_method(D_METHOD("get_id_path", "from_id", "to_id"), &AStarGrid2D::_get_id_path_bind_compat_88047); | ||
ClassDB::bind_compatibility_method(D_METHOD("get_point_path", "from_id", "to_id"), &AStarGrid2D::_get_point_path_bind_compat_88047); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to have an example that uses DEFVAL()
, or at least this needs to be called out that if the previous binding of a method had default values assigned, the compatibility method should have them too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will see if I can find another example that has that to replace this with to make the example more complete
Thank you for the review! Will try to update later today Edit: Cleaned up and fixed the style details, will think about the intro section and update that later today and see if I can find a better example that has old default values and emphasize that the default values of the new method and the old method should be carried over, will see what to add there |
04ed066
to
b03da72
Compare
Unless the change in compatibility is complex the compatibility method should simply call the normal method directly, instead of duplicating | ||
the method, making sure to match the default arguments of the bind. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this to emphasize this part, might be worded better
Will look at the intro section and a more complex example case soon |
3724cb3
to
0934ef2
Compare
There, reduced the intro section and made it clearer that breakgages should be avoided, and added a few |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just re-read this after the latest changes. Looks fantastic! Great work :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the updates, I agree it sounds really good!
Some minor comments, admittedly a bit nitpicky 😀
0934ef2
to
5b9afaa
Compare
5b9afaa
to
3fafd46
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for writing this down and the continuous updates!
I've been planning to do it for a long time and it'll save myself work too as I've largely been the one to instruct how to do this on PRs 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Thanks, that's a great piece of documentation for contributors 🎉 |
Thank you! I'll experiment with some improvements to some of the wording later and we can discuss how to phrase the policy as well here |
Cherry-picked to 4.2 in #9648. |
Basic instructions and examples for using this feature